Skip to content
This repository has been archived by the owner on Jan 24, 2021. It is now read-only.

Nancy.Url no longer contains a fragment property #1421

Merged
merged 4 commits into from
Mar 16, 2014

Conversation

zippy1981
Copy link
Contributor

Unit tests are a bit more discerning too.

@phillip-haydon
Copy link
Member

I would assume the reason this was removed was because a client cannot send a hash to the server, so it doesn't really make any logical sense to include it. You're pretty much never going to get access to it, and if you're redirecting a user as a response you can add it as part of your response anyway.

@zippy1981
Copy link
Contributor Author

@phillip-haydon correct

@phillip-haydon
Copy link
Member

OH Sorry, so you're suggesting we remove it. Ok! I thought you were adding it.

Yup I concur with this change then :D

@grumpydev
Copy link
Member

@zippy1981 have you run all the tests for this? At first glance there's at least one test ( https://github.com/NancyFx/Nancy/pull/1421/files#diff-d81c2d050034607d025b3992cb97ceb2R208 ) that should fail.. you're not setting the fragment, but then asserting that it's there/

@grumpydev
Copy link
Member

The test in question is also called "Should_implicitliy_cast_to_string", and you've added an explicit cast to a string, which makes the test invalid.

@zippy1981
Copy link
Contributor Author

Odd, must have screwed up my last push. Its fixed now.

@phillip-haydon
Copy link
Member

Nope, still failing... Wheres that nope meme when you need it. :D

@prabirshrestha
Copy link
Contributor

Doesn't this change break the web?

var url = new Url { Path = "/", Fragment = "a=b" };
var response = new RedirectResponse(url);
return response;

Also what if the middleware is responsible for modifying the request url and adding a fragment. (Though no one would do this)

@khellang
Copy link
Member

@prabirshrestha After this change, there won't be any Fragment property anymore 😉

@grumpydev
Copy link
Member

RedirectResponse also take a string, not a Url.. the Url class is purely for Nancy's request representation which can never have a fragment.

@prabirshrestha
Copy link
Contributor

@khellang That part I do understand. But then the code I pasted will stop working even though it is valid.

Then Url should not be called Url should be something like NancyRequestUrl. Coz that would be misleading.

@grumpydev
Copy link
Member

@zippy1981 now the PR has commits in it that have nothing to do with the PR because you've merged them in, rather than rebasing.

Also that merge did nothing to fix the failing test :)

@prabirshrestha
Copy link
Contributor

I still don't agree with removing fragment. But if it is removed public static implicit operator Url(Uri uri) should be explicit now as there is loss of data when converting from Uri to Url.

@phillip-haydon
Copy link
Member

@prabirshrestha - because a request cannot contain a fragment then it shouldn't exist on URL, it's a browser thing only and should stay in the browser.

If a user REALLY wants to push a fragment down from the server then I think he should use the magic string value.

Otherwise we should have RequesrUri and ResponseUri to avoid confusion of why Fragment exists.

@prabirshrestha
Copy link
Contributor

because a request cannot contain a fragment then it shouldn't exist on URL, it's a browser thing only and should stay in the browser.

fragment isn't sent by the browser but the server can send fragments down to the browser.

If a user REALLY wants to push a fragment down from the server then I think he should use the magic string value.

That would definitely be the answer if fragment needs to be sent and fragment is removed from Url.

Otherwise we should have RequesrUri and ResponseUri to avoid confusion of why Fragment exists.

I think having comments only telling fragment isn't allowed would make it confusing. Explicit RequestUrl would be better in this case coz that is what it actually is.

@grumpydev
Copy link
Member

There was a loss of data anyway.. fragment was never copied anyway :)

@zippy1981
Copy link
Contributor Author

@grumpydev ok I'll try again after I wipe my laptop

@zippy1981
Copy link
Contributor Author

OK think this will build and I fixed the tests so they do what they say.

@thecodejunkie thecodejunkie added this to the 0.23 milestone Feb 5, 2014
thecodejunkie added a commit that referenced this pull request Mar 16, 2014
Nancy.Url no longer contains a fragment property
@thecodejunkie thecodejunkie merged commit 1e043dc into NancyFx:master Mar 16, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants